Revert "deprecation(groupComparisonPTM): Deprecate data.type parameter#126
Revert "deprecation(groupComparisonPTM): Deprecate data.type parameter#126tonywu1999 merged 1 commit intodevelfrom
Conversation
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
R/groupComparisonPTM.R (1)
127-172:⚠️ Potential issue | 🟠 MajorAdd a hard guard for unsupported label types to avoid undefined models.
If
ptm_label_typeorprotein_label_typeis not"LF"/"TMT", neither branch runs and downstream objects (e.g.,ptm_model,protein_model) can be undefined. Add an explicitelse stop(...)(or upfront validation) for both paths.Proposed fix
if (ptm_label_type == "TMT"){ @@ } else if (ptm_label_type == "LF") { @@ + } else { + stop("`ptm_label_type` must be one of 'LF' or 'TMT'.") } @@ if (protein_label_type == "TMT"){ @@ } else if (protein_label_type == "LF") { @@ + } else { + stop("`protein_label_type` must be one of 'LF' or 'TMT'.") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/groupComparisonPTM.R` around lines 127 - 172, The code lacks guards for unsupported label types causing ptm_model or protein_model to remain undefined; add explicit validation or an else branch that calls stop() when ptm_label_type or protein_label_type is not "TMT" or "LF". Locate the two decision blocks that check ptm_label_type (around the calls to groupComparisonTMT and groupComparison) and protein_label_type (around the calls to groupComparisonTMT and groupComparison) and add an else stop("Unsupported ptm_label_type: ...") / stop("Unsupported protein_label_type: ...") (or perform an upfront check validating ptm_label_type and protein_label_type and aborting), so downstream variables like ptm_model, ptm_model_details, protein_model and protein_model_details are always defined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@R/groupComparisonPTM.R`:
- Around line 94-97: Validate data.type explicitly: if data.type is not NULL,
check if it is one of c("TMT","LF") and if not call stop() with a clear error
message; otherwise (when valid) set ptm_label_type and protein_label_type to
data.type. Replace the current conditional that only assigns on equality with an
explicit validation block using data.type %in% c("TMT","LF") and stop(...) for
invalid values so the code fails fast.
- Around line 246-250: The deprecation warning for the argument data.type in
groupComparisonPTM.R (the if block checking !is.null(data.type) && (data.type ==
"TMT" || data.type == "LF")) must be removed or disabled to match the revert's
compatibility goal; update that block to stop emitting warning (either delete
the warning call or replace it with a no-op comment and/or a
FUTURE_DEPRECATION_FLAG check) so users are not shown the deprecation message
now while preserving the condition logic for future reintroduction.
---
Outside diff comments:
In `@R/groupComparisonPTM.R`:
- Around line 127-172: The code lacks guards for unsupported label types causing
ptm_model or protein_model to remain undefined; add explicit validation or an
else branch that calls stop() when ptm_label_type or protein_label_type is not
"TMT" or "LF". Locate the two decision blocks that check ptm_label_type (around
the calls to groupComparisonTMT and groupComparison) and protein_label_type
(around the calls to groupComparisonTMT and groupComparison) and add an else
stop("Unsupported ptm_label_type: ...") / stop("Unsupported protein_label_type:
...") (or perform an upfront check validating ptm_label_type and
protein_label_type and aborting), so downstream variables like ptm_model,
ptm_model_details, protein_model and protein_model_details are always defined.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2b526817-bdcd-4754-b6cd-6713a75c47c3
📒 Files selected for processing (2)
R/groupComparisonPTM.Rman/groupComparisonPTM.Rd
| if (!is.null(data.type) && (data.type == "TMT" || data.type == "LF")) { | ||
| ptm_label_type = data.type | ||
| protein_label_type = data.type | ||
| } |
There was a problem hiding this comment.
Validate data.type explicitly instead of silently ignoring invalid values.
Line 94 only applies overrides for "LF"/"TMT"; other values are ignored despite docs saying this argument “must be one of” those values. Please fail fast with a clear error.
Proposed fix
+ valid_label_types <- c("LF", "TMT")
+
- if (!is.null(data.type) && (data.type == "TMT" || data.type == "LF")) {
- ptm_label_type = data.type
- protein_label_type = data.type
- }
+ if (!is.null(data.type)) {
+ if (!(data.type %in% valid_label_types)) {
+ stop("`data.type` must be one of 'LF' or 'TMT'.")
+ }
+ ptm_label_type = data.type
+ protein_label_type = data.type
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@R/groupComparisonPTM.R` around lines 94 - 97, Validate data.type explicitly:
if data.type is not NULL, check if it is one of c("TMT","LF") and if not call
stop() with a clear error message; otherwise (when valid) set ptm_label_type and
protein_label_type to data.type. Replace the current conditional that only
assigns on equality with an explicit validation block using data.type %in%
c("TMT","LF") and stop(...) for invalid values so the code fails fast.
| if (!is.null(data.type) && (data.type == "TMT" || data.type == "LF")) { | ||
| warning("DEPRECATION NOTICE: The `data.type` argument is being deprecated. | ||
| Please use `ptm_label_type` and `protein_label_type` instead ahead | ||
| of Release 3.22") | ||
| } |
There was a problem hiding this comment.
Deprecation warning conflicts with this revert’s compatibility goal.
Line 247 still emits a deprecation notice for data.type. That appears inconsistent with the PR objective to postpone deprecation until a future major release, and keeps deprecation behavior user-visible now.
Proposed fix
- if (!is.null(data.type) && (data.type == "TMT" || data.type == "LF")) {
- warning("DEPRECATION NOTICE: The `data.type` argument is being deprecated.
- Please use `ptm_label_type` and `protein_label_type` instead ahead
- of Release 3.22")
- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!is.null(data.type) && (data.type == "TMT" || data.type == "LF")) { | |
| warning("DEPRECATION NOTICE: The `data.type` argument is being deprecated. | |
| Please use `ptm_label_type` and `protein_label_type` instead ahead | |
| of Release 3.22") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@R/groupComparisonPTM.R` around lines 246 - 250, The deprecation warning for
the argument data.type in groupComparisonPTM.R (the if block checking
!is.null(data.type) && (data.type == "TMT" || data.type == "LF")) must be
removed or disabled to match the revert's compatibility goal; update that block
to stop emitting warning (either delete the warning call or replace it with a
no-op comment and/or a FUTURE_DEPRECATION_FLAG check) so users are not shown the
deprecation message now while preserving the condition logic for future
reintroduction.
1 participant
This reverts commit 0630b43. I'm thinking we should wait for a major release (like upgrading MSstatsPTM to 3.x) before doing a change that is not backwards compatibility. Plus we should do this during a release that's not before May Institute.
Motivation and Context
Please include relevant motivation and context of the problem along with a short summary of the solution.
Changes
Please provide a detailed bullet point list of your changes.
Testing
Please describe any unit tests you added or modified to verify your changes.
Checklist Before Requesting a Review
Motivation and Context
This PR reverts commit 0630b43 which deprecated the
data.typeparameter in thegroupComparisonPTM()function. The reversion is necessary to maintain backward compatibility with existing code. The author postpones the removal ofdata.typeuntil a major release (e.g., MSstatsPTM 3.x) rather than removing it in the current development version to avoid breaking changes for existing users who rely on the parameter.Changes
Function signature restoration in
R/groupComparisonPTM.R:data.type = NULLparameter as the second argument in the function signatureptm_label_typedefault fromc("LF", "TMT")(with match.arg validation) back to scalar"LF"protein_label_typedefault fromc("LF", "TMT")(with match.arg validation) back to scalar"LF"baseparameter)data.typevalues toptm_label_typeandprotein_label_typewhendata.typeis provideddata.typewill be deprecated in favor of the two label-type arguments prior to Release 3.22match.arg()validation calls on the label type parametersDocumentation restoration in
man/groupComparisonPTM.Rd:data.type = NULLparameter documentationptm_label_typeandprotein_label_typeback to end of parameter list with scalar defaults"LF"Infrastructure files added:
.Rbuildignore: Standard R package build exclusions.gitignore: Repository exclusions for R artifacts.travis.yml: Travis CI configuration.github/pull_request_template.md: Pull request template with sections for motivation, changes, testing, and checklist.github/workflows/dry-run-build.yml: GitHub Actions workflow for PR builds and checksTesting
The existing tests in
inst/tinytest/test_groupComparisonPTM.Rremain valid and continue to use the explicitptm_label_typeandprotein_label_typeparameters with their scalar values ("LF"and"TMT"). The tests do not require modification as they were already adapted to use the explicit label type parameters rather than thedata.typeparameter.Coding Guidelines
No coding guideline violations are introduced. The reversion maintains consistent parameter handling and documentation practices with the pre-deprecation code structure.